CI add visual regression tests - #672
Conversation
c0ddc08 to
8e84075
Compare
Renders the six dataview classes through quickshow and plot_panels -- for alpha-bearing data, NaNs in the data, and NaNs in the alpha map -- plus non-flatmap views through save_3d_views. Each render is checked against a stored reference, and the two renderers against each other. Four criteria per check, because they are complementary: the mean misses a few pixels moving a long way, the gross fraction misses a broad low-amplitude shift, and SSIM misses a channel permutation. The renderers do not share a pixel grid, so webgl is mapped onto quickflat's frame by a fixed affine first (fit_cross_renderer_affine.py re-derives it), and curvature is un-thresholded on both sides -- which halves the cross-renderer floor at the cost of not covering the default curvature path. Vertex2D is xfailed strictly: its webgl flatmap fails to link its shader (gallantlabgh-714), so it cannot be tested through that path. The test dependency group pins playwright and matplotlib, which between them fix every renderer the references depend on.
Twenty-eight lossless WebP (method=6, quality=100, exact=True) across alpha_dataviews, nan_dataviews, nan_alpha_dataviews and nonflat_views, kept separate from the code that reads them since no diff will explain them. Stored as git LFS pointers from the outset, so the image data never enters this repository's history. Rendered by chromium 151.0.7922.34 and matplotlib 3.10.9, pinned in the test dependency group. Regenerate with REGENERATE_REFERENCE_IMAGES=1 and review the result: regenerating is how a real regression gets silently blessed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A shader that over-allocates vertex attributes compiles but fails to *link*, and the viewer draws a blank canvas without any error (causes e.g. gallantlabgh-714). Three defences. filter_webgl_failures() scans browser output for unambiguous signatures. An event pump is what makes those messages arrive at all: Playwright's sync API dispatches only while something calls into it, and the worker parked in _shutdown_event.wait() for the whole session, so save_3d_views' check was dead code reading a list that stayed empty until teardown. _assert_not_blank inspects the pixels, for a driver that reports nothing at all. save_3d_views now raises RuntimeError naming the view and file, where a broken Vertex2D previously surfaced as "zero-size array to reduction operation minimum" from plot_panels' border trim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@claude review |
|
Claude finished @mvdoc's task in 3m 11s —— View job Review of "CI add visual regression tests"Todo list:
This is a very well-engineered PR — the calibration work behind the tolerances ( 1. Stale doc comment: pw_thread = handle._pw_thread # `handle` is a `WebApp`Per 2. Typo: 3. 4. Minor style: Things that look solid:
No blocking issues found; the three code notes above (1-3) are polish, not correctness bugs. |
- `handle` is the JSMixer, not the WebApp; `handle.server` is the WebApp. - "interactivemode" -> "interactive mode". - Two blank lines before the new test in test_export.py, per PEP8. - Note that browser_errors is cumulative: with a shared handle, one transient failure fails every later test in the class too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8e84075 to
dcd4c24
Compare
|
I pushed new commits that address problem 1, 2, and 4. I think it's not worth fixing problem 3, but I added a note for it. |
mvdoc
left a comment
mvdoc
left a comment
There was a problem hiding this comment.
PR looks good. I ran the test locally and it worked, but it required a bit of fussing with dependencies. Claude gave the following comments. It may be worth adding some pointers in the docs in case we want to run things locally. (The CI tests seem to work fine.)
Two things worth flagging for the PR, both about git-lfs:
Without git-lfs installed, git checkout of this branch aborts outright — smudge filter lfs failed 127 — it doesn't degrade to pointer files, it refuses to check out. I got past it with filters disabled and then fetched the 28 objects through the LFS batch API. Contributors on a machine without git-lfs will hit a hard wall, so the README in reference_images/ should probably say git lfs install is a prerequisite. Related: this repo's LFS bandwidth is now on the hook for every clone that runs the tests.
Also, pytest.ini has testpaths=cortex with coverage on, so a bare uv run pytest on a dev machine with a real filestore in options.cfg will use that subject, not the bundled S1 stub — the reference images only match the stub. On your machine that surfaced as six PermissionErrors; on a writable filestore it would surface as confusing pixel diffs instead. Worth a note in the README, or a conftest that pins the filestore for these tests.
| # No lfs: true on purpose. This installs and imports a wheel, and | ||
| # exclude_package_data keeps the reference images out of wheels, so | ||
| # fetching them here would spend LFS bandwidth on files nothing reads. |
There was a problem hiding this comment.
I'd remove this comment, or at least reword it. "No lfs: true on purpose" is confusing.
| #: so VALIDATE_STATUS is false for want of a run, and getError() 0 is the | ||
| #: absence of an error. Driver shader-info warnings are excluded likewise. | ||
| WEBGL_FAILURE_PATTERNS = ( | ||
| "THREE.WebGLProgram: Could not initialise shader", |
There was a problem hiding this comment.
Can we do a final sweep and use American spelling instead of British?
There was a problem hiding this comment.
or is the error itself with british spelling? did we create that error type, or three.js?
There was a problem hiding this comment.
The error is from three.js and has British spelling. Verbatim from the chromium console:
THREE.WebGLProgram: Could not initialise shader.
There was a problem hiding this comment.
I'll fix the spellings elsewhere. Maybe we should add something in CLAUDE.md to use American spelling conventions?
|
|
||
| #: How often the worker thread calls into Playwright to dispatch queued browser | ||
| #: events; ``browser_errors`` is current to within this interval. | ||
| EVENT_PUMP_INTERVAL = 0.25 |
There was a problem hiding this comment.
is "EVENT PUMP" standard terminology in this pattern or can we rename it to something more intuitive?
| (`CROSS_MAX_MEAN_ABS_DIFF`, `CROSS_MAX_FRACTION_DIFFERING`), with no stored | ||
| fixture. `test_visual_comparison_nonflat_views` runs the reference check only. | ||
|
|
||
| ## The cross-renderer affine correction |
There was a problem hiding this comment.
this is fancy and if it works, fine. but wouldn't have it worked to just crop both images tightly, then match pixel size?
I've only been able to reproduce the LFS checkout errors if 1) git-lfs & its hooks were installed in the repo, 2) the I don't have much experience with LFS, but this seems like a generic LFS issue (inconsistent configuration) and not something worth documenting in this repo. I was still able to check out this branch and run tests if
I can force the tests (either ones just in this PR or all tests) to use the bundled pycortex db. If that's too strong, I can just write a warning at the beginning of |
2 participants
Rendering changes in Pycortex are currently only caught by eye. This PR automatically compares renders from both renderers (quickflat and webgl) against stored reference images, so a change in output fails a test instead of needing to be spotted in a screenshot. It also checks for consistency between the two renderers.
Tests
Four groups of reference images, covering the six public dataview classes through both renderers. (webgl references for
Vertex2Dare not included because it's broken onmain, per gh-714.)alpha_dataviews/Volume/Vertexas a no-alpha baseline,Volume2Dvia the 2D alpha colormapRdBu_r_alpha, the RGB classes via thealpha=keyword. All composite the curvature underlay.nan_dataviews/Vertex2Dxfailed) with NaNs over roughly half the primary data channel. Both renderers should draw those elements fully transparent, falling through to curvature, rather than mapping NaN through the colormap.nan_alpha_dataviews/VolumeRGB/VertexRGBwith the NaNs in the alpha map instead. A separate path: alpha is not colour-mapped but used directly as a blend weight, so the NaN reaches the compositing arithmetic.nonflat_views/VolumeandVertexon the inflated and fiducial surfaces. webgl only —quickshowrenders flatmaps and nothing else — so these get the reference check and no cross-renderer leg.The images are tracked with git LFS and should be regenerated whenever a rendering change is blessed.
The within-renderer checks run at tighter tolerances than the cross-renderer checks, since the two renderers differ by a fixed affine transform. (
fit_cross_renderer_affine.pyre-derives the ideal transform.)WebGL errors now fail loudly. Previously, WebGL console messages were not being caught properly due to buffering. This PR correctly propagates console error into pycortex. (This change would have caught gh-714.)
A blank render now fails loudly.
save_3d_viewsalso now raises on a blank view, naming the view and the file.What it catches
Three real bugs:
nanmeandefault. The mean difference (0.21 against a limit of 2.0) and the fraction of pixels moving more than 16 (0.68% against 2%) both pass. Only the gross-difference fraction and SSIM catch it — which is why there are four criteria and not one.main: the canvas comes back blank, nothing raises, and the png is still written.Notes for reviewers
tmp_path. There is no side-by-side composite; the two files are easier to flip between than one wide image.git lfs pullif the reference images arrive as 130-byte text stubs. The tests skip rather than fail on that, and on the images being absent.xfail(strict=True). When the fix (Vertex2D objects do not render in webviewer #714) merges they turn XPASS.matplotlibandplaywrightare now pinned in dev environments for consistency with the reference images.install_from_wheel.ymldeliberately does not, since wheels exclude them.cortex/tests/reference_images/README.mdcovers regeneration and provenance.